Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes: #3862; Added an example Python Support case study #3882

Merged
merged 19 commits into from
Nov 8, 2024

Conversation

himanshumahajan138
Copy link
Contributor

@himanshumahajan138 himanshumahajan138 commented Oct 31, 2024

Pull Request

Added an example Python Support case study
Fixes: #3862

Description

Added Python Support for Mill From basic installation to multiple supports like virtual environment, mypy integration , pex integration

Related Issues

Checklist

  • Python-hello example
  • Python-modules example
  • Python-dependencies example
  • Python-mypy support example
  • Python-pex Support example
  • Documentation Updated

Additional Notes: (Code Copy and Plagiarism is Strictly Prohibited in case of Bounty)

Reminder: Contributions must be original or appropriately attributed. Strict action may be taken by the community for cheating and coping the code from some PR for Bounty or plagiarized code to maintain integrity within the community.

@lihaoyi
Copy link
Member

lihaoyi commented Oct 31, 2024

Please clean this up to follow the Typescript example documentation. Your code and documentation is far too verbose, far too repetitive, and generally not sufficiently simplified to be suitable for educational purposes. I cannot fix every detail for you, you need to go and read through those examples yourself and fix your code and documentation to match

@himanshumahajan138
Copy link
Contributor Author

Ok @lihaoyi sir, Thanks for reviewing

My silly mistake made one test fail its just f string error 😔

No worries i will update the code and documentation

@lihaoyi lihaoyi marked this pull request as draft November 1, 2024 22:54
@himanshumahajan138 himanshumahajan138 marked this pull request as ready for review November 2, 2024 20:54
@himanshumahajan138
Copy link
Contributor Author

@lihaoyi Sir Final Testing has been done...

  • Documentation Updated (Written by me 😅)
  • Code Logic Updated as per TypeScript Example
  • combined moduleDeps, virtualEnv & libraryDeps (coz they are dependent to each other)
  • All Tests Passed and Code is checked thrice...

Please pardon me if anything missed or done in wrong way...

@himanshumahajan138
Copy link
Contributor Author

@lihaoyi Sir Let me tell you one thing that i have seen actually i installed Ubuntu in WSL and when i run it for the first time it just only provides python which is installed with only setuptools and nothing else (no pip , no venv and others )

So when i searched on internet i came to know that we have to install pip and venv using sudo apt install python3-pip and sudo apt install python3-venv

but now here comes the problem of permissions we cant get sudo for every task and even this is not the good practise

so to tackle this problem i have used official guide for installing pip for users without requiring sudo permission and this will be installed for the user rather than the sudo

i think this is a good approach, rest if you say i will follow your way...

@lefou
Copy link
Member

lefou commented Nov 3, 2024

A Mill build process should not install anything outside of the project directory, unless it's some kind of cache like coursier. We should just depend on installed tools and provide meaningful error messages if they are missing. We can provide installation/setup support in some external module, e.g. build.PythonModule/installPip, but that should be purely optional.

@himanshumahajan138
Copy link
Contributor Author

himanshumahajan138 commented Nov 3, 2024

A Mill build process should not install anything outside of the project directory, unless it's some kind of cache like coursier. We should just depend on installed tools and provide meaningful error messages if they are missing. We can provide installation/setup support in some external module, e.g. build.PythonModule/installPip, but that should be purely optional.

But Sir what about github actions tests ?

i think they will fail (not sure)

@himanshumahajan138
Copy link
Contributor Author

himanshumahajan138 commented Nov 3, 2024

Sir Please see this image i just installed the ubuntu-24.04 version and when i tried these commands you can see that it don't comes with pip and venv

Moreover, i am not downloading anything outside the T.dest actually i just downloaded get-pip.py in T.dest and then using this we installed pip for user originally that's the official way

image

@lihaoyi
Copy link
Member

lihaoyi commented Nov 3, 2024

You should be able to use python3 -m pip and python3 -m venv without needing installation

@himanshumahajan138
Copy link
Contributor Author

ok then let me recall the updates again:

  • no need to install pip and virtualenv
  • mypy will be added in first example (5-hello-python) change in compile function
  • no need for activating venv
  • no need for all sources
  • and other minor updates...

Thanks For Review Sir (This community is filled with helpful members❤️)

@lihaoyi
Copy link
Member

lihaoyi commented Nov 3, 2024

@himanshumahajan138 yep! I expect this will take multiple rounds of back and forth review to polish into shape, but it's moving in the right direction so I think we'll eventually get there

@lihaoyi
Copy link
Member

lihaoyi commented Nov 6, 2024

@himanshumahajan138 took a pass, most of the comments apply to all the examples.

Getting pretty close I think, looks a lot better than when we started!

@himanshumahajan138
Copy link
Contributor Author

himanshumahajan138 commented Nov 6, 2024

@lihaoyi sir, like i have used type checking in every example is that a good and right way

Moreover i will update all of these reviews for every example

@lihaoyi
Copy link
Member

lihaoyi commented Nov 6, 2024

Yes having typechecking in every example is right. The examples are meant to build upon each other, so we would expect them to get longer and more complex as it progresses down the list

@himanshumahajan138
Copy link
Contributor Author

@lihaoyi Sir, finally I have Updated the Code as per reviews and now I think we can move further in review or merging part

Hope you Like it...

Thanks for Reviews Sir...


*/

// Uncomment the Result variable line(Line Number:13) in the `src/main.py` code to see `typeCheck` error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than using instructions here, we should use sed to uncomment it in the Usage block, and then run ./mill typeCheck again to show the error that is generated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually sir i want to show error also but the problem is that if i write code for error then it will fail in ci test as this will me a major error from mypy side so how can i do this

please Guide me a little Bit Sir...

@lihaoyi
Copy link
Member

lihaoyi commented Nov 7, 2024

Took a review pass. Many of the comments apply to the following docs and examples as well

@himanshumahajan138
Copy link
Contributor Author

@lihaoyi Sir, Ready for Next Reviews

what i changed in last updates:

  • updated documentation and some code
  • updated scripts and shorten them
  • rewrite the documentation
  • removed irrelevant and repeative codes and docs

Thanks For Reviews Sir...

@himanshumahajan138
Copy link
Contributor Author

@lihaoyi Sir is there something wrong with ci tests this unknown type of test case failed (not even related to my code changes) ?

unknowingly, My PR Got ❌ Tag...

@lihaoyi
Copy link
Member

lihaoyi commented Nov 8, 2024

@himanshumahajan138 I think this looks good. Just need to run ./mill mill.scalalib.scalafmt.ScalafmtModule/ to reformat everything and we can merge it

@himanshumahajan138
Copy link
Contributor Author

himanshumahajan138 commented Nov 8, 2024

Actually sir before my last commit ci don't run for .mill build files but now it do for these files also and its was my fault that i didn't checked it...
No worries let me update and then finalize it...

Thanks sir for your guidance and reviews...

@himanshumahajan138
Copy link
Contributor Author

AND HERE WE GO🎉

Everything Ready for Merge Lets make this happen...

Thanks A Lot Sir ❤️
(To be Honest You have Changed my Life I was Nothing before contributing to Mill and now I am Confident with my Skills and alot financially also)

@lihaoyi lihaoyi merged commit 0c6186c into com-lihaoyi:main Nov 8, 2024
14 checks passed
@lefou lefou added this to the 0.12.2 milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an example Python Support case study (500USD bounty)
3 participants